fix: sqlframe false positives in compliance checks#2011
fix: sqlframe false positives in compliance checks#2011FBruzzesi merged 4 commits intonarwhals-dev:mainfrom
Conversation
camriddell
changed the title
There was a problem hiding this comment.
@MarcoGorelli for the coverage is there a preference among
- Add
sqlframeto the pytest GitHub CI - Remove this test
pragma: nocoverthe tests
There was a problem hiding this comment.
Just FYI, I am taking a look for how to fully support sqlframe - it's still missing a few features (not many) to pass all our tests. I would say that for now it's ok to pragma: no cover it
There was a problem hiding this comment.
definitely interested in testing sqlframe in CI, last time i checked it was too far to being passing - but they've resolved a lot of issues recently, so if we can run them in ci now then that would be great
else for now we can no cover the test
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @camriddell 🚀 Left a couple of suggestion and (maybe) addressed your question
There was a problem hiding this comment.
Just FYI, I am taking a look for how to fully support sqlframe - it's still missing a few features (not many) to pass all our tests. I would say that for now it's ok to pragma: no cover it
narwhals/translate.py
Outdated
| ) | ||
|
|
||
| # SQLFrame | ||
| # This one needs checking before extensions as `hasattr` always returns `True`. |
MarcoGorelli
left a comment
MarcoGorelli
left a comment
There was a problem hiding this comment.
you're a magician 🪄
thanks! this may fix a cudf.pandas issue too!
|
pending on #2011 (comment) 😉 |
|
failing downstream tests seem to be unrelated. I know that |
|
Cool thanks! pointblank also just merged a new feature, we can update its dependency on another PR |
4 participants
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below
In the
narwhals.from_nativefunction,sqlframehad to be explicitly checked before theis_compliant*checks due to false positives fromhasattr(sqlframe_df, "__narwhals_dataframe__").Instead we can use
inspect.getattr_static(thanks for the suggestion @dangotbanned) to avoid this addition and we can move thesqlframespecific code to be in line with the other backends.I also added a test to ensure
sqlframecases are handled properly.I removed a test that will break in the future, this test was passing due to a side effect caused by
hasattron apandasDataFrame enabling the use of nonhashable values as column names (raised as an issue in pandas BUG:Index.drop_duplicates()is inconsistent for unhashable values pandas-dev/pandas#60925)